[clang] Informative error for lifetimebound in decl-spec #118567

emaxx-google · 2024-12-04T00:37:45Z

Emit a bit more informative error when the [[clang::lifetimebound]] attribute is wrongly appearing on a decl-spec:

'lifetimebound' attribute only applies to parameters and implicit
object parameters

instead of:

'lifetimebound' attribute cannot be applied to types

The new error is also consistent with the diagnostic emitted when the attribute is misplaced in other parts of a declarator.

llvmbot · 2024-12-04T00:38:22Z

@llvm/pr-subscribers-clang

Author: Maksim Ivanov (emaxx-google)

Changes

Emit a bit more informative error when the [[clang::lifetimebound]] attribute is wrongly appearing on a decl-spec:

"'lifetimebound' attribute only applies to parameters and implicit
object parameters",

instead of:

"'lifetimebound' attribute cannot be applied to types".

The new error is also consistent with the diagnostic emitted when the attribute is misplaced in other parts of a declarator.

Full diff: https://github.com/llvm/llvm-project/pull/118567.diff

2 Files Affected:

(modified) clang/lib/Parse/ParseDecl.cpp (+8-2)
(modified) clang/test/SemaCXX/attr-lifetimebound.cpp (+2-2)

diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 937a94b02458c6..caa1a12297bc01 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -3703,8 +3703,14 @@ void Parser::ParseDeclarationSpecifiers(
           // We reject AT_LifetimeBound and AT_AnyX86NoCfCheck, even though they
           // are type attributes, because we historically haven't allowed these
           // to be used as type attributes in C++11 / C23 syntax.
-          if (PA.isTypeAttr() && PA.getKind() != ParsedAttr::AT_LifetimeBound &&
-              PA.getKind() != ParsedAttr::AT_AnyX86NoCfCheck)
+          if (PA.getKind() == ParsedAttr::AT_LifetimeBound) {
+            Diag(PA.getLoc(), diag::err_attribute_wrong_decl_type_str)
+                << PA << PA.isRegularKeywordAttribute()
+                << "parameters and implicit object parameters";
+            PA.setInvalid();
+            continue;
+          }
+          if (PA.isTypeAttr() && PA.getKind() != ParsedAttr::AT_AnyX86NoCfCheck)
             continue;
           Diag(PA.getLoc(), diag::err_attribute_not_type_attr)
               << PA << PA.isRegularKeywordAttribute();
diff --git a/clang/test/SemaCXX/attr-lifetimebound.cpp b/clang/test/SemaCXX/attr-lifetimebound.cpp
index c7abec61873efb..896793f9966666 100644
--- a/clang/test/SemaCXX/attr-lifetimebound.cpp
+++ b/clang/test/SemaCXX/attr-lifetimebound.cpp
@@ -10,7 +10,7 @@ namespace usage_invalid {
     static int *static_class_member() [[clang::lifetimebound]]; // expected-error {{static member function has no implicit object parameter}}
     int *explicit_object(this A&) [[clang::lifetimebound]]; // expected-error {{explicit object member function has no implicit object parameter}}
     int attr_on_var [[clang::lifetimebound]]; // expected-error {{only applies to parameters and implicit object parameters}}
-    int [[clang::lifetimebound]] attr_on_int; // expected-error {{cannot be applied to types}}
+    int [[clang::lifetimebound]] attr_on_int; // expected-error {{'lifetimebound' attribute only applies to parameters and implicit object parameters}}
     int * [[clang::lifetimebound]] attr_on_int_ptr; // expected-error {{'lifetimebound' attribute only applies to parameters and implicit object parameters}}
     int * [[clang::lifetimebound]] * attr_on_int_ptr_ptr; // expected-error {{'lifetimebound' attribute only applies to parameters and implicit object parameters}}
     int (* [[clang::lifetimebound]] attr_on_func_ptr)(); // expected-error {{'lifetimebound' attribute only applies to parameters and implicit object parameters}}
@@ -19,7 +19,7 @@ namespace usage_invalid {
   int *attr_with_param(int &param [[clang::lifetimebound(42)]]); // expected-error {{takes no arguments}}
 
   void attr_on_ptr_arg(int * [[clang::lifetimebound]] ptr); // expected-error {{'lifetimebound' attribute only applies to parameters and implicit object parameters}}
-  static_assert((int [[clang::lifetimebound]]) 12); // expected-error {{cannot be applied to types}}
+  static_assert((int [[clang::lifetimebound]]) 12); // expected-error {{'lifetimebound' attribute only applies to parameters and implicit object parameters}}
   int* attr_on_unnamed_arg(const int& [[clang::lifetimebound]]); // expected-error {{'lifetimebound' attribute only applies to parameters and implicit object parameters}}
   template <typename T>
   int* attr_on_template_ptr_arg(T * [[clang::lifetimebound]] ptr); // expected-error {{'lifetimebound' attribute only applies to parameters and implicit object parameters}}

emaxx-google · 2024-12-04T00:39:48Z

@hokein @ilya-biryukov

cor3ntin · 2024-12-04T05:13:14Z

clang/lib/Parse/ParseDecl.cpp

+          if (PA.getKind() == ParsedAttr::AT_LifetimeBound) {
+            Diag(PA.getLoc(), diag::err_attribute_wrong_decl_type_str)
+                << PA << PA.isRegularKeywordAttribute()
+                << "parameters and implicit object parameters";


Why are we not using %select for that?

Beside, is "implicit object parameters" sufficiently understandable? - or should we say "member functions"?

Why are we not using %select for that?

Can you explain a bit more? IIUC, you're suggesting using the err_attribute_wrong_decl_type the one with the "%select"? That is possible, but then we'll need to add a new Kind in AttributeDeclKind.

Beside, is "implicit object parameters" sufficiently understandable? - or should we say "member functions"?

I think the implicit object parameter is fine. "member functions" is not entirely correct, the attribute is on the implicit object parameter even though it is applied to a function type in the implementation.

I would like to second @cor3ntin's comment. We should aim to have all message strings in one place (i.e. the .td file), it makes searching for error messages much easier.

However, err_attribute_wrong_decl_type_str in particular seems to be using strings in the source code (1, 2, 3).
So I think this change is consistent with that, and I think we should just land it and replace this with a follow-up instead (@emaxx-google would you be willing to do it?)

Adding another AttributeDeclKind seems off, given that it's a enumeration used primarily by parser. But adding a new enumeration for select (that maps to AttributeDeclKind for the known values and adds a few more) seems like a good path forward.

I'm happy to follow up with the change!

I do see that err_attribute_wrong_decl_type_str uses that same method in other places which is unfortunate. It makes looking for diagnostics a lot harder.

clang/lib/Parse/ParseDecl.cpp

hokein

The change looks good to me.

cor3ntin

LGTM as long as there is a follow up :)

Emit a bit more informative error when the `[[clang::lifetimebound]]` attribute is wrongly appearing on a decl-spec: "'lifetimebound' attribute only applies to parameters and implicit object parameters", instead of: "'lifetimebound' attribute cannot be applied to types". The new error is also consistent with the diagnostic emitted when the attribute is misplaced in other parts of a declarator.

emaxx-google · 2025-01-10T17:37:04Z

#122473 is the promised follow-up for the diag.

Emit a bit more informative error when the `[[clang::lifetimebound]]` attribute is wrongly appearing on a decl-spec: ``` 'lifetimebound' attribute only applies to parameters and implicit object parameters ``` instead of: ``` 'lifetimebound' attribute cannot be applied to types ``` The new error is also consistent with the diagnostic emitted when the attribute is misplaced in other parts of a declarator.

A cleanup follow-up to #118501 and #118567.

A cleanup follow-up to llvm#118501 and llvm#118567.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 4, 2024

cor3ntin reviewed Dec 4, 2024

View reviewed changes

hokein reviewed Dec 5, 2024

View reviewed changes

clang/lib/Parse/ParseDecl.cpp Outdated Show resolved Hide resolved

hokein approved these changes Dec 5, 2024

View reviewed changes

cor3ntin approved these changes Dec 13, 2024

View reviewed changes

emaxx-google added 2 commits January 10, 2025 12:35

refactor diag code

80c0b2c

emaxx-google force-pushed the lifetimebound-warn-cannot-be-applied-to-types branch from cfc2018 to 80c0b2c Compare January 10, 2025 12:35

hokein merged commit 6b12272 into llvm:main Jan 10, 2025
6 of 7 checks passed

emaxx-google mentioned this pull request Jan 10, 2025

[clang] Refactor attr diagnostics to use %select #122473

Merged

ilya-biryukov pushed a commit that referenced this pull request Jan 13, 2025

[clang] Refactor attr diagnostics to use %select (#122473)

41a94de

A cleanup follow-up to #118501 and #118567.

kazutakahirata pushed a commit to kazutakahirata/llvm-project that referenced this pull request Jan 13, 2025

[clang] Refactor attr diagnostics to use %select (llvm#122473)

9a9b71c

A cleanup follow-up to llvm#118501 and llvm#118567.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang] Informative error for lifetimebound in decl-spec #118567

[clang] Informative error for lifetimebound in decl-spec #118567

emaxx-google commented Dec 4, 2024 •

edited

Loading

Uh oh!

llvmbot commented Dec 4, 2024

Uh oh!

emaxx-google commented Dec 4, 2024

Uh oh!

cor3ntin Dec 4, 2024

Uh oh!

hokein Dec 5, 2024

Uh oh!

ilya-biryukov Dec 5, 2024

Uh oh!

emaxx-google Dec 5, 2024

Uh oh!

shafik Dec 20, 2024

Uh oh!

Uh oh!

hokein left a comment

Uh oh!

cor3ntin left a comment

Uh oh!

Uh oh!

emaxx-google commented Jan 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[clang] Informative error for lifetimebound in decl-spec #118567

[clang] Informative error for lifetimebound in decl-spec #118567

Conversation

emaxx-google commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 4, 2024

Uh oh!

emaxx-google commented Dec 4, 2024

Uh oh!

cor3ntin Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

hokein Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

ilya-biryukov Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

emaxx-google Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

shafik Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hokein left a comment

Choose a reason for hiding this comment

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emaxx-google commented Jan 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

emaxx-google commented Dec 4, 2024 •

edited

Loading